From a468236a4bfa8fa002bb96eb207c477fb0f23379 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Wed, 14 Jan 2015 09:58:43 -0800 Subject: [PATCH] Vastly improve configuration error messages This commit seeks to improve error messages with respect to a misconfigured Cargo. This is done with a few principles and architectural changes: * All error messages related to configuration should now mention the file in question. * All error messages should print the key in question which was in error. * Loading configuration values is now centralized in the `Config` structure, providing precisely one location where error message originate from. The messages were previously strewn and duplicated about. * The `Config` structure now provides typed `get_foo` methods to load various configuration keys of various types. --- src/cargo/ops/cargo_compile.rs | 102 ++++++---------- src/cargo/ops/cargo_new.rs | 62 +++------- src/cargo/ops/registry.rs | 46 +------ src/cargo/util/config.rs | 213 +++++++++++++++++++-------------- tests/test_bad_config.rs | 89 ++++++++++++++ tests/test_cargo_compile.rs | 2 +- tests/tests.rs | 1 + 7 files changed, 275 insertions(+), 240 deletions(-) create mode 100644 tests/test_bad_config.rs diff --git a/src/cargo/ops/cargo_compile.rs b/src/cargo/ops/cargo_compile.rs index ec002d943..d4827c37d 100644 --- a/src/cargo/ops/cargo_compile.rs +++ b/src/cargo/ops/cargo_compile.rs @@ -32,8 +32,8 @@ use core::{Source, SourceId, PackageSet, Package, Target, PackageId}; use core::resolver::Method; use ops::{self, BuildOutput, ExecEngine}; use sources::{PathSource}; -use util::config::{Config, ConfigValue}; -use util::{CargoResult, config, internal, human, ChainError, profile}; +use util::config::Config; +use util::{CargoResult, internal, human, ChainError, profile}; /// Contains informations about how a package should be compiled. pub struct CompileOptions<'a, 'b: 'a> { @@ -188,87 +188,59 @@ fn source_ids_from_config(config: &Config, cur_path: Path) fn scrape_build_config(config: &Config, jobs: Option, target: Option) -> CargoResult { - let configs = try!(config.values()); let mut base = ops::BuildConfig { jobs: jobs.unwrap_or(os::num_cpus() as u32), requested_target: target.clone(), ..Default::default() }; - let target_config = match configs.get("target") { - None => return Ok(base), - Some(target) => try!(target.table().chain_error(|| { - internal("invalid configuration for the key `target`") - })), - }; - - base.host = try!(scrape_target_config(target_config, config.rustc_host())); + base.host = try!(scrape_target_config(config, config.rustc_host())); base.target = match target.as_ref() { - Some(triple) => try!(scrape_target_config(target_config, &triple[])), + Some(triple) => try!(scrape_target_config(config, &triple[])), None => base.host.clone(), }; Ok(base) } -fn scrape_target_config(target: &HashMap, - triple: &str) +fn scrape_target_config(config: &Config, triple: &str) -> CargoResult { - let target = match target.get(&triple.to_string()) { - None => return Ok(Default::default()), - Some(target) => try!(target.table().chain_error(|| { - internal(format!("invalid configuration for the key \ - `target.{}`", triple)) - })), - }; + let key = format!("target.{}", triple); + let ar = try!(config.get_string(&format!("{}.ar", key)[])); + let linker = try!(config.get_string(&format!("{}.linker", key)[])); let mut ret = ops::TargetConfig { - ar: None, - linker: None, + ar: ar.map(|p| p.0), + linker: linker.map(|p| p.0), overrides: HashMap::new(), }; - for (k, v) in target.iter() { - match k.as_slice() { - "ar" | "linker" => { - let v = try!(v.string().chain_error(|| { - internal(format!("invalid configuration for key `{}`", k)) - })).0.to_string(); - if k.as_slice() == "linker" { - ret.linker = Some(v); - } else { - ret.ar = Some(v); - } - } - lib_name => { - let table = try!(v.table().chain_error(|| { - internal(format!("invalid configuration for the key \ - `target.{}.{}`", triple, lib_name)) - })); - let mut output = BuildOutput { - library_paths: Vec::new(), - library_links: Vec::new(), - metadata: Vec::new(), - }; - for (k, v) in table.iter() { - let v = try!(v.string().chain_error(|| { - internal(format!("invalid configuration for the key \ - `target.{}.{}.{}`", triple, lib_name, - k)) - })).0; - if k.as_slice() == "rustc-flags" { - let whence = format!("in `target.{}.{}.rustc-flags`", - triple, lib_name); - let whence = whence.as_slice(); - let (paths, links) = try!( - BuildOutput::parse_rustc_flags(v.as_slice(), whence) - ); - output.library_paths.extend(paths.into_iter()); - output.library_links.extend(links.into_iter()); - } else { - output.metadata.push((k.to_string(), v.to_string())); - } - } - ret.overrides.insert(lib_name.to_string(), output); + let table = match try!(config.get_table(&key[])) { + Some((table, _)) => table, + None => return Ok(ret), + }; + for (lib_name, _) in table.into_iter() { + if lib_name == "ar" || lib_name == "linker" { continue } + + let mut output = BuildOutput { + library_paths: Vec::new(), + library_links: Vec::new(), + metadata: Vec::new(), + }; + let key = format!("{}.{}", key, lib_name); + let table = try!(config.get_table(&key[])).unwrap().0; + for (k, _) in table.into_iter() { + let key = format!("{}.{}", key, k); + let (v, path) = try!(config.get_string(&key[])).unwrap(); + if k == "rustc-flags" { + let whence = format!("in `{}` (in {:?})", key, path); + let (paths, links) = try!( + BuildOutput::parse_rustc_flags(v.as_slice(), &whence[]) + ); + output.library_paths.extend(paths.into_iter()); + output.library_links.extend(links.into_iter()); + } else { + output.metadata.push((k, v)); } } + ret.overrides.insert(lib_name, output); } Ok(ret) diff --git a/src/cargo/ops/cargo_new.rs b/src/cargo/ops/cargo_new.rs index 9649fffd4..7c8e52fe6 100644 --- a/src/cargo/ops/cargo_new.rs +++ b/src/cargo/ops/cargo_new.rs @@ -153,50 +153,24 @@ fn discover_author() -> CargoResult<(String, Option)> { } fn global_config(config: &Config) -> CargoResult { - let user_configs = try!(config.values()); - let mut cfg = CargoNewConfig { - name: None, - email: None, - version_control: None, - }; - let cargo_new = match user_configs.get("cargo-new") { - None => return Ok(cfg), - Some(target) => try!(target.table().chain_error(|| { - internal("invalid configuration for the key `cargo-new`") - })), - }; - cfg.name = match cargo_new.get("name") { - None => None, - Some(name) => { - Some(try!(name.string().chain_error(|| { - internal("invalid configuration for key `cargo-new.name`") - })).0.to_string()) - } - }; - cfg.email = match cargo_new.get("email") { - None => None, - Some(email) => { - Some(try!(email.string().chain_error(|| { - internal("invalid configuration for key `cargo-new.email`") - })).0.to_string()) + let name = try!(config.get_string("cargo-new.name")).map(|s| s.0); + let email = try!(config.get_string("cargo-new.email")).map(|s| s.0); + let vcs = try!(config.get_string("cargo-new.vcs")); + + let vcs = match vcs.as_ref().map(|p| (&p.0[], &p.1)) { + Some(("git", _)) => Some(VersionControl::Git), + Some(("hg", _)) => Some(VersionControl::Hg), + Some(("none", _)) => Some(VersionControl::NoVcs), + Some((s, p)) => { + return Err(internal(format!("invalid configuration for key \ + `cargo-new.vcs`, unknown vcs `{}` \ + (found in {:?})", s, p))) } + None => None }; - cfg.version_control = match cargo_new.get("vcs") { - None => None, - Some(vcs) => { - let vcs_str = try!(vcs.string().chain_error(|| { - internal("invalid configuration for key `cargo-new.vcs`") - })).0; - let version_control = match vcs_str.as_slice() { - "git" => VersionControl::Git, - "hg" => VersionControl::Hg, - "none"=> VersionControl::NoVcs, - _ => return Err(internal("invalid configuration for key `cargo-new.vcs`")), - }; - - Some(version_control) - } - }; - - Ok(cfg) + Ok(CargoNewConfig { + name: name, + email: email, + version_control: vcs, + }) } diff --git a/src/cargo/ops/registry.rs b/src/cargo/ops/registry.rs index 28b636f37..567218cdf 100644 --- a/src/cargo/ops/registry.rs +++ b/src/cargo/ops/registry.rs @@ -16,7 +16,7 @@ use core::manifest::ManifestMetadata; use ops; use sources::{PathSource, RegistrySource}; use util::config; -use util::{CargoResult, human, internal, ChainError, ToUrl}; +use util::{CargoResult, human, ChainError, ToUrl}; use util::config::{Config, ConfigValue, Location}; use util::important_paths::find_root_manifest_for_cwd; @@ -131,29 +131,8 @@ fn transmit(pkg: &Package, tarball: &Path, registry: &mut Registry) } pub fn registry_configuration(config: &Config) -> CargoResult { - let configs = try!(config.values()); - let registry = match configs.get("registry") { - None => return Ok(RegistryConfig { index: None, token: None }), - Some(registry) => try!(registry.table().chain_error(|| { - internal("invalid configuration for the key `registry`") - })), - }; - let index = match registry.get("index") { - None => None, - Some(index) => { - Some(try!(index.string().chain_error(|| { - internal("invalid configuration for key `index`") - })).0.to_string()) - } - }; - let token = match registry.get("token") { - None => None, - Some(token) => { - Some(try!(token.string().chain_error(|| { - internal("invalid configuration for key `token`") - })).0.to_string()) - } - }; + let index = try!(config.get_string("registry.index")).map(|p| p.0); + let token = try!(config.get_string("registry.token")).map(|p| p.0); Ok(RegistryConfig { index: index, token: token }) } @@ -193,21 +172,8 @@ pub fn http_handle(config: &Config) -> CargoResult { /// Favor cargo's `http.proxy`, then git's `http.proxy`, then finally a /// HTTP_PROXY env var. pub fn http_proxy(config: &Config) -> CargoResult> { - let configs = try!(config.values()); - match configs.get("http") { - Some(http) => { - let http = try!(http.table().chain_error(|| { - internal("invalid configuration for the key `http`") - })); - match http.get("proxy") { - Some(proxy) => { - return Ok(Some(try!(proxy.string().chain_error(|| { - internal("invalid configuration for key `http.proxy`") - })).0.to_string())) - } - None => {}, - } - } + match try!(config.get_string("http.proxy")) { + Some((s, _)) => return Ok(Some(s)), None => {} } match git2::Config::open_default() { @@ -235,7 +201,7 @@ pub fn registry_login(config: &Config, token: String) -> CargoResult<()> { map.insert("token".to_string(), ConfigValue::String(token, p)); config::set_config(config, Location::Global, "registry", - ConfigValue::Table(map)) + ConfigValue::Table(map, Path::new("."))) } pub struct OwnersOptions { diff --git a/src/cargo/util/config.rs b/src/cargo/util/config.rs index 544eff51f..4dc87ddfc 100644 --- a/src/cargo/util/config.rs +++ b/src/cargo/util/config.rs @@ -93,8 +93,84 @@ impl<'a> Config<'a> { pub fn cwd(&self) -> &Path { &self.cwd } + pub fn get(&self, key: &str) -> CargoResult> { + let vals = try!(self.values()); + let mut parts = key.split('.').enumerate(); + let mut val = match vals.get(parts.next().unwrap().1) { + Some(val) => val, + None => return Ok(None), + }; + for (i, part) in parts { + match *val { + CV::Table(ref map, _) => { + val = match map.get(part) { + Some(val) => val, + None => return Ok(None), + } + } + CV::String(_, ref path) | + CV::List(_, ref path) | + CV::Boolean(_, ref path) => { + let idx = key.split('.').take(i) + .fold(0, |n, s| n + s.len()) + i - 1; + let key_so_far = key.slice_to(idx); + return Err(human(format!("expected table for configuration \ + key `{}`, but found {} in {:?}", + key_so_far, val.desc(), path))); + } + } + } + Ok(Some(val.clone())) + } + + pub fn get_string(&self, key: &str) -> CargoResult> { + match try!(self.get(key)) { + Some(CV::String(i, path)) => Ok(Some((i, path))), + Some(val) => self.expected("string", key, val), + None => Ok(None), + } + } + + pub fn get_table(&self, key: &str) + -> CargoResult, Path)>> { + match try!(self.get(key)) { + Some(CV::Table(i, path)) => Ok(Some((i, path))), + Some(val) => self.expected("table", key, val), + None => Ok(None), + } + } + + pub fn expected(&self, ty: &str, key: &str, val: CV) -> CargoResult { + val.expected(ty).map_err(|e| { + human(format!("invalid configuration for key `{}`\n{}", key, e)) + }) + } + fn load_values(&self) -> CargoResult<()> { - *self.values.borrow_mut() = try!(all_configs(&self.cwd)); + let mut cfg = CV::Table(HashMap::new(), Path::new(".")); + + try!(walk_tree(&self.cwd, |mut file| { + let path = file.path().clone(); + let contents = try!(file.read_to_string()); + let table = try!(cargo_toml::parse(contents.as_slice(), + &path).chain_error(|| { + human(format!("could not parse TOML configuration in `{:?}`", + path)) + })); + let toml = toml::Value::Table(table); + let value = try!(CV::from_toml(&path, toml).chain_error(|| { + human(format!("failed to load TOML configuration from `{:?}`", + path)) + })); + try!(cfg.merge(value)); + Ok(()) + }).chain_error(|| human("Couldn't load Cargo configuration"))); + + + *self.values.borrow_mut() = match cfg { + CV::Table(map, _) => map, + _ => unreachable!(), + }; Ok(()) } } @@ -108,8 +184,8 @@ pub enum Location { #[derive(Eq,PartialEq,Clone,RustcDecodable)] pub enum ConfigValue { String(String, Path), - List(Vec<(String, Path)>), - Table(HashMap), + List(Vec<(String, Path)>, Path), + Table(HashMap, Path), Boolean(bool, Path), } @@ -119,15 +195,15 @@ impl fmt::Show for ConfigValue { CV::String(ref string, ref path) => { write!(f, "{} (from {})", string, path.display()) } - CV::List(ref list) => { + CV::List(ref list, ref path) => { try!(write!(f, "[")); for (i, &(ref s, ref path)) in list.iter().enumerate() { if i > 0 { try!(write!(f, ", ")); } try!(write!(f, "{} (from {})", s, path.display())); } - write!(f, "]") + write!(f, "] (from {:?})", path) } - CV::Table(ref table) => write!(f, "{:?}", table), + CV::Table(ref table, _) => write!(f, "{:?}", table), CV::Boolean(b, ref path) => { write!(f, "{} (from {})", b, path.display()) } @@ -139,11 +215,11 @@ impl Encodable for ConfigValue { fn encode(&self, s: &mut S) -> Result<(), S::Error> { match *self { CV::String(ref string, _) => string.encode(s), - CV::List(ref list) => { + CV::List(ref list, _) => { let list: Vec<&String> = list.iter().map(|s| &s.0).collect(); list.encode(s) } - CV::Table(ref table) => table.encode(s), + CV::Table(ref table, _) => table.encode(s), CV::Boolean(b, _) => b.encode(s), } } @@ -158,17 +234,21 @@ impl ConfigValue { Ok(CV::List(try!(val.into_iter().map(|toml| { match toml { toml::Value::String(val) => Ok((val, path.clone())), - _ => Err(internal("")), + v => Err(human(format!("expected string but found {} \ + in list", v.type_str()))), } - }).collect::>()))) + }).collect::>()), path.clone())) } toml::Value::Table(val) => { Ok(CV::Table(try!(val.into_iter().map(|(key, value)| { - let value = try!(CV::from_toml(path, value)); + let value = try!(CV::from_toml(path, value).chain_error(|| { + human(format!("failed to parse key `{}`", key)) + })); Ok((key, value)) - }).collect::>()))) + }).collect::>()), path.clone())) } - _ => return Err(internal("")) + v => return Err(human(format!("found TOML configuration value of \ + unknown type `{}`", v.type_str()))) } } @@ -176,11 +256,11 @@ impl ConfigValue { match (self, from) { (&mut CV::String(..), CV::String(..)) | (&mut CV::Boolean(..), CV::Boolean(..)) => {} - (&mut CV::List(ref mut old), CV::List(ref mut new)) => { + (&mut CV::List(ref mut old, _), CV::List(ref mut new, _)) => { let new = mem::replace(new, Vec::new()); old.extend(new.into_iter()); } - (&mut CV::Table(ref mut old), CV::Table(ref mut new)) => { + (&mut CV::Table(ref mut old, _), CV::Table(ref mut new, _)) => { let new = mem::replace(new, HashMap::new()); for (key, value) in new.into_iter() { match old.entry(key) { @@ -201,32 +281,28 @@ impl ConfigValue { pub fn string(&self) -> CargoResult<(&str, &Path)> { match *self { CV::String(ref s, ref p) => Ok((s.as_slice(), p)), - _ => Err(internal(format!("expected a string, but found a {}", - self.desc()))), + _ => self.expected("string"), } } - pub fn table(&self) -> CargoResult<&HashMap> { + pub fn table(&self) -> CargoResult<(&HashMap, &Path)> { match *self { - CV::Table(ref table) => Ok(table), - _ => Err(internal(format!("expected a table, but found a {}", - self.desc()))), + CV::Table(ref table, ref p) => Ok((table, p)), + _ => self.expected("table"), } } pub fn list(&self) -> CargoResult<&[(String, Path)]> { match *self { - CV::List(ref list) => Ok(list.as_slice()), - _ => Err(internal(format!("expected a list, but found a {}", - self.desc()))), + CV::List(ref list, _) => Ok(list.as_slice()), + _ => self.expected("list"), } } pub fn boolean(&self) -> CargoResult<(bool, &Path)> { match *self { CV::Boolean(b, ref p) => Ok((b, p)), - _ => Err(internal(format!("expected a bool, but found a {}", - self.desc()))), + _ => self.expected("bool"), } } @@ -239,17 +315,31 @@ impl ConfigValue { } } + pub fn definition_path(&self) -> &Path { + match *self { + CV::Boolean(_, ref p) | + CV::String(_, ref p) | + CV::List(_, ref p) | + CV::Table(_, ref p) => p + } + } + + fn expected(&self, wanted: &str) -> CargoResult { + Err(internal(format!("expected a {}, but found a {} in {:?}", + wanted, self.desc(), self.definition_path()))) + } + fn into_toml(self) -> toml::Value { match self { CV::Boolean(s, _) => toml::Value::Boolean(s), CV::String(s, _) => toml::Value::String(s), - CV::List(l) => toml::Value::Array(l - .into_iter() - .map(|(s, _)| toml::Value::String(s)) - .collect()), - CV::Table(l) => toml::Value::Table(l.into_iter() - .map(|(k, v)| (k, v.into_toml())) - .collect()), + CV::List(l, _) => toml::Value::Array(l + .into_iter() + .map(|(s, _)| toml::Value::String(s)) + .collect()), + CV::Table(l, _) => toml::Value::Table(l.into_iter() + .map(|(k, v)| (k, v.into_toml())) + .collect()), } } } @@ -260,55 +350,6 @@ fn homedir() -> Option { return cargo_home.or(user_home); } -pub fn get_config(pwd: Path, key: &str) -> CargoResult { - find_in_tree(&pwd, |file| extract_config(file, key)).map_err(|_| - human(format!("`{}` not found in your configuration", key))) -} - -pub fn all_configs(pwd: &Path) -> CargoResult> { - let mut cfg = CV::Table(HashMap::new()); - - try!(walk_tree(pwd, |mut file| { - let path = file.path().clone(); - let contents = try!(file.read_to_string()); - let table = try!(cargo_toml::parse(contents.as_slice(), &path).chain_error(|| { - internal(format!("could not parse Toml manifest; path={}", - path.display())) - })); - let value = try!(CV::from_toml(&path, toml::Value::Table(table))); - try!(cfg.merge(value)); - Ok(()) - }).chain_error(|| human("Couldn't load Cargo configuration"))); - - - match cfg { - CV::Table(map) => Ok(map), - _ => unreachable!(), - } -} - -fn find_in_tree(pwd: &Path, mut walk: F) -> CargoResult - where F: FnMut(File) -> CargoResult -{ - let mut current = pwd.clone(); - - loop { - let possible = current.join(".cargo").join("config"); - if possible.exists() { - let file = try!(File::open(&possible)); - - match walk(file) { - Ok(res) => return Ok(res), - _ => () - } - } - - if !current.pop() { break; } - } - - Err(internal("")) -} - fn walk_tree(pwd: &Path, mut walk: F) -> CargoResult<()> where F: FnMut(File) -> CargoResult<()> { @@ -342,14 +383,6 @@ fn walk_tree(pwd: &Path, mut walk: F) -> CargoResult<()> Ok(()) } -fn extract_config(mut file: File, key: &str) -> CargoResult { - let contents = try!(file.read_to_string()); - let mut toml = try!(cargo_toml::parse(contents.as_slice(), file.path())); - let val = try!(toml.remove(&key.to_string()).chain_error(|| internal(""))); - - CV::from_toml(file.path(), val) -} - pub fn set_config(cfg: &Config, loc: Location, key: &str, value: ConfigValue) -> CargoResult<()> { // TODO: There are a number of drawbacks here diff --git a/tests/test_bad_config.rs b/tests/test_bad_config.rs new file mode 100644 index 000000000..8ecc23f0a --- /dev/null +++ b/tests/test_bad_config.rs @@ -0,0 +1,89 @@ +use support::{project, execs}; +use hamcrest::assert_that; + +fn setup() {} + +test!(bad1 { + let foo = project("foo") + .file("Cargo.toml", r#" + [package] + name = "foo" + version = "0.0.0" + authors = [] + "#) + .file("src/lib.rs", "") + .file(".cargo/config", r#" + target = "foo" + "#); + assert_that(foo.cargo_process("build").arg("-v"), + execs().with_status(101).with_stderr("\ +expected table for configuration key `target`, but found string in [..]config +")); +}); + +test!(bad2 { + let foo = project("foo") + .file("Cargo.toml", r#" + [package] + name = "foo" + version = "0.0.0" + authors = [] + "#) + .file("src/lib.rs", "") + .file(".cargo/config", r#" + [http] + proxy = 3 + "#); + assert_that(foo.cargo_process("publish").arg("-v"), + execs().with_status(101).with_stderr("\ +Couldn't load Cargo configuration + +Caused by: + failed to load TOML configuration from `[..]config` + +Caused by: + failed to parse key `http` + +Caused by: + failed to parse key `proxy` + +Caused by: + found TOML configuration value of unknown type `integer` +")); +}); + +test!(bad3 { + let foo = project("foo") + .file("Cargo.toml", r#" + [package] + name = "foo" + version = "0.0.0" + authors = [] + "#) + .file("src/lib.rs", "") + .file(".cargo/config", r#" + [http] + proxy = true + "#); + assert_that(foo.cargo_process("publish").arg("-v"), + execs().with_status(101).with_stderr("\ +invalid configuration for key `http.proxy` +expected a string, but found a boolean in [..]config +")); +}); + +test!(bad4 { + let foo = project("foo") + .file(".cargo/config", r#" + [cargo-new] + name = false + "#); + assert_that(foo.cargo_process("new").arg("-v").arg("foo"), + execs().with_status(101).with_stderr("\ +Failed to create project `foo` at `[..]` + +Caused by: + invalid configuration for key `cargo-new.name` +expected a string, but found a boolean in [..]config +")); +}); diff --git a/tests/test_cargo_compile.rs b/tests/test_cargo_compile.rs index fb81b74a8..91707762a 100644 --- a/tests/test_cargo_compile.rs +++ b/tests/test_cargo_compile.rs @@ -1401,7 +1401,7 @@ test!(bad_cargo_config { Couldn't load Cargo configuration Caused by: - could not parse Toml manifest; path=[..] + could not parse TOML configuration; path=[..] Caused by: could not parse input as TOML diff --git a/tests/tests.rs b/tests/tests.rs index 72fde609e..96a4049ae 100644 --- a/tests/tests.rs +++ b/tests/tests.rs @@ -25,6 +25,7 @@ macro_rules! test { ) } +mod test_bad_config; mod test_cargo; mod test_cargo_bench; mod test_cargo_build_auth; -- 2.30.2